feat(rust/sedona-proj): Let ST_Transform() handle 3D coordinates #544
Conversation
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
Thank you!
This is a great addition as is...I suggested a few test cases but I'm not sure all of my suggestions work and we can do those later. In particular our CrsTransform isn't really set up to handle changes in dimensions. We could implement that change when we implement ST_Force2D/3DZM(), too so that we can implement those functions using CrsTransform.
Also linking #47 here (this PR will close that).
| eng = eng.create_or_skip() | ||
| eng.assert_query_result( | ||
| "SELECT ST_Transform(ST_GeomFromText('POINT Z (1 1 1)'), 'EPSG:4326', 'EPSG:4978')", | ||
| "POINT Z (6376201.805927448 111297.016517882 110568.792276973)", | ||
| wkt_precision=9, | ||
| ) |
There was a problem hiding this comment.
This particular transformation is a great test case and an interesting one because it is 2D -> 3D. Does 2D input (e.g., POINT (-64 45)) work? I believe you can query the number of axes of a CRS to handle this:
https://github.com/apache/sedona-db/blob/main/c/sedona-proj/src/proj_dyn_bindgen.rs#L127-L128
...although we can do that later because I think 3D->3D is what the current code is set up to handle.
Another good test would be 3D -> 2D (the inverse of this transform would be good) and 3D -> 3D like EPSG:4326+EPSG:5701 (lon/lat with Z in meters) to EPSG:4326+EPSG:8050 (lon/lat with Z in feet). You might have to get pyproj to render those to PROJJSON to get them to work in ST_Transform because our internal deserialize_crs() doesn't know how to parse compound CRSes yet.
Do you mean conversions between 2D CRS and 3D CRS should change the dimension of the geometry? In PostGIS, I don't see such changes. |
I think that it should although you certainly don't have to implement that here and it is probably worth digging into why PostGIS doesn't do this before deciding that's what should happen. There is significantly less precedent around 3D/4D/spatiotemporal CRS handling and I don't think this is one of the things we should perfectly match PostGIS if we can do a better job. (Or maybe our alternative behaviour should have a function with a different name while we sort out what that behaviour should be). |
|
Thanks for clarifying. I'm not immediately sure what's the right behavior, but at least I agree with this part:
|
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
Great! Let's merge this and deal with changes in dimensions elsewhere ( #554 ).
Thank you!
|
Thanks for merging! I'll comment there once I've formed an opinion on this. |
2 participants
As
Proj::transform()already transforms 4D, the implementation is fairly simple.I guess the hard part is to find proper test cases...